Skip to content

[fix][io] KCA: 'desanitize' topic name for the pulsar's ctx calls#19756

Merged
eolivelli merged 1 commit intoapache:masterfrom
dlg99:kca-sanitize-bq
Mar 9, 2023
Merged

[fix][io] KCA: 'desanitize' topic name for the pulsar's ctx calls#19756
eolivelli merged 1 commit intoapache:masterfrom
dlg99:kca-sanitize-bq

Conversation

@dlg99
Copy link
Contributor

@dlg99 dlg99 commented Mar 9, 2023

Motivation

Some kafka connectors use topic name to e.g. create table names but pulsar's topic URI does not comply with requirements of the table names. This problems either handled by the connector or by KCA if sanitizeTopicName is set to true.
Some kafka connectors use pause/resume API, in that case sanitized name has to be converted back to original URI to pass to Pulsar's components.

Modifications

Provided a way to resolve original topic URI.

Added unit test.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added unit test.

Does this pull request potentially affect one of the following parts:

NO

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: dlg99#10

@dlg99 dlg99 self-assigned this Mar 9, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 9, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #19756 (dc57e44) into master (af1360f) will increase coverage by 35.61%.
The diff coverage is 82.35%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19756       +/-   ##
=============================================
+ Coverage     26.40%   62.02%   +35.61%     
- Complexity     6427    25758    +19331     
=============================================
  Files          1597     1848      +251     
  Lines        123682   135882    +12200     
  Branches      13511    14954     +1443     
=============================================
+ Hits          32658    84279    +51621     
+ Misses        86336    43810    -42526     
- Partials       4688     7793     +3105     
Flag Coverage Δ
inttests 24.53% <ø> (-0.04%) ⬇️
systests 25.12% <ø> (?)
unittests 59.37% <82.35%> (+41.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ache/pulsar/io/kafka/connect/KafkaConnectSink.java 73.06% <70.00%> (ø)
...r/io/kafka/connect/PulsarKafkaSinkTaskContext.java 64.03% <100.00%> (ø)
...ce/ConsistentHashingStickyKeyConsumerSelector.java 76.92% <0.00%> (-3.85%) ⬇️
...in/java/org/apache/pulsar/common/api/AuthData.java 71.42% <0.00%> (ø)
.../apache/pulsar/broker/namespace/LookupOptions.java 87.50% <0.00%> (ø)
.../apache/pulsar/common/naming/SystemTopicNames.java 55.55% <0.00%> (ø)
...apache/pulsar/common/util/SafeCollectionUtils.java 0.00% <0.00%> (ø)
...pache/pulsar/common/configuration/BindAddress.java 22.22% <0.00%> (ø)
...lsar/client/impl/conf/ReaderConfigurationData.java 81.39% <0.00%> (ø)
...r/client/admin/internal/data/AuthPoliciesImpl.java 65.21% <0.00%> (ø)
... and 1348 more

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eolivelli eolivelli merged commit d4930a3 into apache:master Mar 9, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 9, 2023
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants